Skip to content

KEP-5589: Initial draft for gogo protobuf removal #5590

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

KEP-5589: Initial draft for gogo protobuf removal #5590

wants to merge 3 commits into from
+689 −0

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Sep 30, 2025

  • One-line PR description: Initial draft for gogo protobuf removal

/sig api-machinery architecture
/cc @deads2k @jpbetz @dims

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 30, 2025
This was referenced Sep 30, 2025
Open
Open
Open
@serathius
Copy link
Contributor

serathius commented Sep 30, 2025 •
edited
Loading

/cc @ahrtr
Might be relevant for etcd

@k8s-ci-robot k8s-ci-robot requested a review from ahrtr September 30, 2025 16:48
dims
dims reviewed Sep 30, 2025
View reviewed changes
dims
dims reviewed Sep 30, 2025
View reviewed changes
dims
dims reviewed Sep 30, 2025
View reviewed changes
dims
dims reviewed Sep 30, 2025
View reviewed changes
dims
dims reviewed Sep 30, 2025
View reviewed changes
@liggitt
Copy link
Member Author

liggitt commented Sep 30, 2025

/cc @ahrtr

I'm not sure the approach taken here is particularly relevant to etcd and etcd-io/etcd#14533 ... etcd does treat the .proto file as the canonical API definition, and does fully generate go types from .proto without the wild Go-rewriting steps Kubernetes does.

aojea
aojea reviewed Sep 30, 2025
View reviewed changes
aojea
aojea reviewed Sep 30, 2025
View reviewed changes
aojea
aojea reviewed Sep 30, 2025
View reviewed changes
aojea
aojea reviewed Sep 30, 2025
View reviewed changes
@aojea
Copy link
Member

aojea commented Sep 30, 2025

LGTM modulo one comment

We have to get rid of this sword of Damocles hanging over the project

pohly
pohly reviewed Oct 1, 2025
View reviewed changes
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

saschagrunert
saschagrunert approved these changes Oct 1, 2025
View reviewed changes
jpbetz
jpbetz approved these changes Oct 1, 2025
View reviewed changes
Copy link
Contributor

@jpbetz jpbetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoke with @liggitt earlier about this approach. It's pragmatic. I'm onboard.

/approve

/hold
For approval from @deads2k

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, liggitt, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2025
benluddy
benluddy reviewed Oct 1, 2025
View reviewed changes
@ahrtr
Copy link
Member

ahrtr commented Oct 3, 2025

/cc @ahrtr

I'm not sure the approach taken here is particularly relevant to etcd and etcd-io/etcd#14533 ... etcd does treat the .proto file as the canonical API definition, and does fully generate go types from .proto without the wild Go-rewriting steps Kubernetes does.

etcd has some minor rewrite (converting v1 messages to v2), refer to https://github.com/etcd-io/etcd/blob/682d4fb5e281bb4f987e9350d5896d3bebdfadf4/scripts/genproto.sh#L177-L210

Could you point me to the rewrite & related doc Kubernetes does?

ahrtr
ahrtr reviewed Oct 3, 2025
View reviewed changes
keps/sig-api-machinery/5589-gogo-dependency/README.md
Comment on lines +448 to +449
The changes in this KEP are to code generation, and primarily result in removal of unused generated code.
There is no runtime enablement / disablement of these changes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes perfect sense!

But it seems that introducing new interfaces might be unnecessarily complicate this KEP; and it might be better to consider it as a separate KEP.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's no new interfaces, we're referencing interfaces defined within gogo packages currently ... we're redefining those interfaces locally instead

@liggitt
Copy link
Member Author

liggitt commented Oct 3, 2025

etcd has some minor rewrite (converting v1 messages to v2), refer to https://github.com/etcd-io/etcd/blob/682d4fb5e281bb4f987e9350d5896d3bebdfadf4/scripts/genproto.sh#L177-L210

Could you point me to the rewrite & related doc Kubernetes does?

Ours is here, after running the gogo generation step:
https://github.com/kubernetes/code-generator/blob/v0.34.0/cmd/go-to-protobuf/protobuf/cmd.go#L289-L293

The rewrite is defined here: https://github.com/kubernetes/code-generator/blob/master/cmd/go-to-protobuf/protobuf/parser.go#L80

@liggitt
Copy link
Member Author

liggitt commented Oct 3, 2025

etcd has some minor rewrite (converting v1 messages to v2), refer to https://github.com/etcd-io/etcd/blob/682d4fb5e281bb4f987e9350d5896d3bebdfadf4/scripts/genproto.sh#L177-L210

that's unfortunate, I'm pretty sure gogo messages aren't fully compliant v2 messages

@ahrtr
Copy link
Member

ahrtr commented Oct 3, 2025

etcd has some minor rewrite (converting v1 messages to v2), refer to https://github.com/etcd-io/etcd/blob/682d4fb5e281bb4f987e9350d5896d3bebdfadf4/scripts/genproto.sh#L177-L210

that's unfortunate, I'm pretty sure gogo messages aren't fully compliant v2 messages

The rewrite is just to support REST API generated by grpc-gateway (so the core etcd's grpc API isn't impacted), and we have enough test cases to ensure nothing is broken.

Sorry to derail the discussion of the KEP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pohly pohly pohly left review comments

@dims dims dims left review comments

@saschagrunert saschagrunert saschagrunert approved these changes

@BenTheElder BenTheElder BenTheElder left review comments

@aojea aojea aojea left review comments

@jpbetz jpbetz jpbetz approved these changes

@deads2k deads2k Awaiting requested review from deads2k

+2 more reviewers

@benluddy benluddy benluddy left review comments

@ahrtr ahrtr ahrtr left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.